-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enforce file extension in import #380
Conversation
🦋 Changeset detectedLatest commit: 416ed42 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
import { getNextWrappingIndex, getNextNonDisabledIndex } from '../../utils'; | ||
import { getHighlightedIndexOnOpen, getDefaultValue } from '../utils'; | ||
import { getItemIndexByCharacterKey } from './utils'; | ||
import { getHighlightedIndexOnOpen, getDefaultValue } from '../utils.js'; | ||
import { getItemIndexByCharacterKey } from './utils.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why does the following not have the .js
extension?
import { getNextWrappingIndex, getNextNonDisabledIndex } from '../../utils';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it only enforces it for files which it can find/actually exist.
So in this case, the file doesn't exist (since this file is just one of the fixtures files picked out of the downshift project, and other files in that project weren't included).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, interesting.
From the changeset:
History of extension-less imports:
require()
paths it will look in several places for the module, until it finds one:ex:
require('./foo')
checks for:./foo
./foo.js
./foo.json
./foo.node
./foo/package.json
withmain
field./foo/index.js
require()
in browsers with bundlers (Browserify and then Webpack)import
/export
, by using the same internal mechanisms asrequire()
, meaning that they preserved the resolution algorithm used byrequire()
.import './foo'
resolves like<a href="./foo"
). There is no mechanism for rewriting paths like./foo
->./foo.js
(except if the server provides redirects)parcel
,esbuild
,rollup-plugin-node-resolve
,typescript
) followed what webpack and browserify did, attempting to minimize compatibility issues when switching between bundlers.require
thanimport
.import
statements by settingmoduleResolution
tonode12
. When that option is set, TypeScript will no longer resolveimport './foo'
to./foo.js
or./foo.ts
, and it will throw an error (same as Node). The TS team decided that in order to import a file calledfoo.ts
, you must use a.js
import:import './foo.js'
, even though the filefoo.js
does not exist. TS will not let you useimport './foo.ts'
, even though./foo.ts
exists. The rationale is that if TS was to rewrite the import path during code emit from.ts
to.js
, it would be a functionality change to the JS code, which they avoid with a ten-foot pole. (I don't think this decision was the right one, but it is what it is).In other words, syntax like
import './foo'
is syntax that was never really valid/specified. It is a mishmash of the ES import syntax and the CommonJS module specifier syntax.require('./foo')
is valid andimport './foo.js'
is valid.To help move our projects towards supporting native-node ESM and native-browser ESM, this PR enforces the use of file extensions in all path imports. That means it will switch
require('./foo')
torequire('./foo.js')
even though that change is not needed. The goal is consistency across all imports/requires, even in projects that use CommonJS or a bundler that shims in CommonJS resolution into ESM.